HIVE-28669: Deadlock found when TxnStoreMutex trying to acquireLock #5585

dengzhhu653 · 2024-12-20T10:25:29Z

What changes were proposed in this pull request?

Resolving the Deadlock when the back db is MySQL

Why are the changes needed?

By default MySQL default isolation level is REPEATABLE-READ, for update in this isolation will hold the gap lock, if multiple clients are trying to for update and then insert into the same gap, it cloud cause the deadlock.

Does this PR introduce any user-facing change?

A new hive.metastore.have.multiple.leaders, if it's false, then the same housekeeping tasks will not leverage the db mutex to block each other.

Is the change a dependency upgrade?

No

How was this patch tested?

Testing the PR locally, querying from mysql information_schema.INNODB_TRX table, showing that trx_isolation_level of the new trx is 'READ COMMITTED'

Aggarwal-Raghav · 2024-12-21T16:09:37Z

Based on the comments on JIRA and I have tested this patch on my local and haven't seen the error stacktrace, hence
LGTM +1 (non-binding)

ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java

deniskuzZ · 2025-01-07T08:53:51Z

@dengzhhu653, before HIVE-27481 we always used READ_COMMITTED isolation level in TxnHandler:

@Override
  public LockHandle acquireLock(String key) throws MetaException {
    /**
     * The implementation here is a bit kludgey but done so that code exercised by unit tests
     * (which run against Derby which has no support for select for update) is as similar to
     * production code as possible.
     * In particular, with Derby we always run in a single process with a single metastore and
     * the absence of For Update is handled via a Semaphore.  The later would strictly speaking
     * make the SQL statements below unnecessary (for Derby), but then they would not be tested.
     */
    Connection dbConn = null;
    Statement stmt = null;
    ResultSet rs = null;
    boolean needToCloseConn = true;
    try {
      try {
        String sqlStmt = sqlGenerator.addForUpdateClause("SELECT \"MT_COMMENT\", \"MT_KEY2\" FROM \"AUX_TABLE\" WHERE \"MT_KEY1\"=" + quoteString(key));
        lockInternal();
        dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED, connPoolMutex);
        stmt = dbConn.createStatement();
        LOG.debug("About to execute SQL: {}", sqlStmt);
        rs = stmt.executeQuery(sqlStmt);

if that is changed to default, should we restore the READ_COMMITTED?

...store/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDummyMutex.java

deniskuzZ · 2025-01-07T13:06:13Z

...erver/src/main/java/org/apache/hadoop/hive/metastore/txn/jdbc/TransactionContextManager.java

-  public TransactionContext getNewTransaction(int propagation) {
-    TransactionContext context = new TransactionContext(realTransactionManager.getTransaction(
-        new DefaultTransactionDefinition(propagation)), this);
+  public TransactionContext getNewTransaction(Transactional transactional) {


why not create a parameterless method and use Propagation.REQUIRED as default?

this transactional is retrieved from the method annotation,
https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnStore.java#L281
each method might have its own definition, though we only care about the isolation level and propagation now

by default, the isolation is default, and we don't specify another level elsewhere.
we don't introduce the isolation() in the TxnStore, the level can be specified via:

@Transactional(value = POOL_TX, isolation = Isolation.READ_COMMITTED, propagation = Propagation.REQUIRED, timeout = 30) GetOpenTxnsResponse getOpenTxns() throws MetaException;

how about:

public TransactionContext getNewTransaction(int propagation, int isolation) { DefaultTransactionDefinition transactionDefinition = new DefaultTransactionDefinition(propagation); transactionDefinition.setIsolationLevel((isolation != Isolation.DEFAULT.value()) ? isolation : ISOLATION_READ_COMMITTED); TransactionContext context = new TransactionContext( realTransactionManager.getTransaction(transactionDefinition), this); contexts.set(context); return context; } public TransactionContext getNewTransaction(int propagation) { return getNewTransaction(propagation, ISOLATION_READ_COMMITTED); } .... context = jdbcResource.getTransactionManager().getNewTransaction( transactional.propagation().value(), transactional.isolation().value());

so, you decided to get rid of the isolation level? that's fine, but in that case, why do we need 2nd method without the propagation arg?

done, removed the 2nd method

...c/main/java/org/apache/hadoop/hive/metastore/txn/jdbc/functions/AbortCompactionFunction.java

...e/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/leader/LeaderElection.java

dengzhhu653 · 2025-01-07T13:39:04Z

@dengzhhu653, before HIVE-27481 we always used READ_COMMITTED isolation level in TxnHandler:

@Override
  public LockHandle acquireLock(String key) throws MetaException {
    /**
     * The implementation here is a bit kludgey but done so that code exercised by unit tests
     * (which run against Derby which has no support for select for update) is as similar to
     * production code as possible.
     * In particular, with Derby we always run in a single process with a single metastore and
     * the absence of For Update is handled via a Semaphore.  The later would strictly speaking
     * make the SQL statements below unnecessary (for Derby), but then they would not be tested.
     */
    Connection dbConn = null;
    Statement stmt = null;
    ResultSet rs = null;
    boolean needToCloseConn = true;
    try {
      try {
        String sqlStmt = sqlGenerator.addForUpdateClause("SELECT \"MT_COMMENT\", \"MT_KEY2\" FROM \"AUX_TABLE\" WHERE \"MT_KEY1\"=" + quoteString(key));
        lockInternal();
        dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED, connPoolMutex);
        stmt = dbConn.createStatement();
        LOG.debug("About to execute SQL: {}", sqlStmt);
        rs = stmt.executeQuery(sqlStmt);

if that is changed to default, should we restore the READ_COMMITTED?

I have made the default as the READ_COMMITTED when calling the TxnStore's method unless it has specified the level:
https://github.com/apache/hive/pull/5585/files#diff-63125a22ca15ef76a1e6b5a52d230e9b24c816cbd15442e3ba640da2f4c167d0R113
https://github.com/apache/hive/pull/5585/files#diff-080bfb902495302473126b9827a086cf164e63d8742c3b80082469b01b405a48R66-R67

deniskuzZ

LGTM +1, pending tests

sonarqubecloud · 2025-01-08T02:03:04Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

HIVE-28669: Deadlock found when TxnStoreMutex trying to acquireLock

205d484

asf-ci-hive added tests pending tests unstable and removed tests pending labels Dec 20, 2024

Indhumathi27 reviewed Dec 23, 2024

View reviewed changes

ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java Show resolved Hide resolved

Indhumathi27 reviewed Dec 23, 2024

View reviewed changes

ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java Show resolved Hide resolved

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Jan 6, 2025

dengzhhu653 requested review from deniskuzZ and saihemanth-cloudera January 7, 2025 05:17

deniskuzZ reviewed Jan 7, 2025

View reviewed changes

...store/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnDummyMutex.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 7, 2025

View reviewed changes

...c/main/java/org/apache/hadoop/hive/metastore/txn/jdbc/functions/AbortCompactionFunction.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 7, 2025

View reviewed changes

...e/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/leader/LeaderElection.java Outdated Show resolved Hide resolved

asf-ci-hive added tests pending and removed tests passed labels Jan 7, 2025

review-1

d4fedb1

dengzhhu653 force-pushed the HIVE-28669 branch from 0390bf3 to d4fedb1 Compare January 7, 2025 14:56

asf-ci-hive added tests failed and removed tests pending labels Jan 7, 2025

review-2

95e1003

deniskuzZ approved these changes Jan 7, 2025

View reviewed changes

asf-ci-hive added tests pending and removed tests failed tests pending labels Jan 7, 2025

asf-ci-hive added the tests failed label Jan 7, 2025

review-3

de9eac9

asf-ci-hive added tests pending and removed tests failed labels Jan 8, 2025

asf-ci-hive added tests passed and removed tests pending labels Jan 8, 2025

dengzhhu653 merged commit 1836187 into apache:master Jan 8, 2025
4 checks passed

dengzhhu653 deleted the HIVE-28669 branch January 8, 2025 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-28669: Deadlock found when TxnStoreMutex trying to acquireLock #5585

HIVE-28669: Deadlock found when TxnStoreMutex trying to acquireLock #5585

dengzhhu653 commented Dec 20, 2024

Aggarwal-Raghav commented Dec 21, 2024

deniskuzZ commented Jan 7, 2025

deniskuzZ Jan 7, 2025

dengzhhu653 Jan 7, 2025

dengzhhu653 Jan 7, 2025

deniskuzZ Jan 7, 2025 •

edited

Loading

dengzhhu653 Jan 7, 2025

deniskuzZ Jan 7, 2025 •

edited

Loading

dengzhhu653 Jan 8, 2025

dengzhhu653 commented Jan 7, 2025

deniskuzZ left a comment

sonarqubecloud bot commented Jan 8, 2025

HIVE-28669: Deadlock found when TxnStoreMutex trying to acquireLock #5585

HIVE-28669: Deadlock found when TxnStoreMutex trying to acquireLock #5585

Conversation

dengzhhu653 commented Dec 20, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

Aggarwal-Raghav commented Dec 21, 2024

deniskuzZ commented Jan 7, 2025

deniskuzZ Jan 7, 2025

Choose a reason for hiding this comment

dengzhhu653 Jan 7, 2025

Choose a reason for hiding this comment

dengzhhu653 Jan 7, 2025

Choose a reason for hiding this comment

deniskuzZ Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

dengzhhu653 Jan 7, 2025

Choose a reason for hiding this comment

deniskuzZ Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

dengzhhu653 Jan 8, 2025

Choose a reason for hiding this comment

dengzhhu653 commented Jan 7, 2025

deniskuzZ left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 8, 2025

Quality Gate passed

deniskuzZ Jan 7, 2025 •

edited

Loading

deniskuzZ Jan 7, 2025 •

edited

Loading